-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Optimize ReadOnlyTernaryTree for System.Private.Xml #60076
Conversation
Tagging subscribers to this area: @dotnet/area-system-xml Issue Detailsnull
|
Thanks @kronic I'll let area owner review this. One thing that would really help reviewers in future would be to keep formatting and functional changes separate. Either a separate PR (if large) or at least a separate commit 😄 |
@danmoseley I will consider in the future |
src/libraries/System.Private.Xml/src/System/Xml/Core/ReadOnlyTernaryTree.cs
Show resolved
Hide resolved
src/libraries/System.Private.Xml/src/System/Xml/Core/ReadOnlyTernaryTree.cs
Show resolved
Hide resolved
Hi @kronic, we would need a bit more information on this PR. Would it be possible to 1) provide a synopsis of the performance optimizations you are attempting to make and 2) share benchmarks demonstrating the improvements? Thanks. |
src/libraries/System.Private.Xml/src/System/Xml/Core/HtmlEncodedRawTextWriter.cs
Show resolved
Hide resolved
before System.Private.Xml.dll size 3 087 360 byte
after System.Private.Xml.dll size 3 086 848 byte
#region
using System.IO;
using System.Xml;
using BenchmarkDotNet.Attributes;
#endregion
namespace MicroBenchmarks.libraries.System.Xml.Linq
{
[BenchmarkCategory(Categories.Libraries)]
public class Pref_XmlHTml
{
private XmlWriterSettings _xmlWriterSettings;
private XmlWriterSettings _xmlWriterSettings1;
[GlobalSetup]
public void Setup()
{
_xmlWriterSettings = new XmlWriterSettings
{
Indent = false
};
_xmlWriterSettings1 = new XmlWriterSettings
{
Indent = true
};
typeof(XmlWriterSettings).GetProperty(nameof(XmlWriterSettings.OutputMethod))!.SetValue
(_xmlWriterSettings, XmlOutputMethod.Html);
typeof(XmlWriterSettings).GetProperty(nameof(XmlWriterSettings.OutputMethod))!.SetValue
(_xmlWriterSettings1, XmlOutputMethod.Html);
}
[Benchmark]
public void WriteStartElement()
{
using var xmlWriter = XmlWriter.Create(Stream.Null, _xmlWriterSettings);
xmlWriter.WriteStartElement(string.Empty, "WriteStartElement", string.Empty);
}
[Benchmark]
public void WriteStartElementIdent()
{
using var xmlWriter = XmlWriter.Create(Stream.Null, _xmlWriterSettings1);
xmlWriter.WriteStartElement(string.Empty, "WriteStartElementIdent", string.Empty);
}
}
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thank you! Will only kick off the outerloop tests to double check no regressions
/azp list |
This comment has been minimized.
This comment has been minimized.
/azp run runtime-libraries-coreclr outerloop-windows |
Azure Pipelines successfully started running 1 pipeline(s). |
I'll close/reopen PR since something broke with the CI checks |
/azp run runtime-libraries-coreclr outerloop-windows |
Azure Pipelines successfully started running 1 pipeline(s). |
I've marked this PR as "needs more info" for now. We'd like to better understand the risk/reward for the series of XML changes. See #61773 (comment). Thanks! |
I gave my answer Comment |
@jeffhandley this particular PR looks like a good optimization to me. In general as we've mentioned in the other PRs: please create an uber issue with motivation and share the approach you're taking so that we can also give you pointers how to achieve performance goals you have and also bucketize types of changes you're making so that any mistakes are easier to catch and fix |
/azp run |
You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list. |
/azp run runtime-libraries-coreclr outerloop-windows |
Azure Pipelines successfully started running 1 pipeline(s). |
test failures are unrelated |
No description provided.